Add --no-abi flag to sncast declare, declare-from & deploy#4269
Add --no-abi flag to sncast declare, declare-from & deploy#4269die-herdplatte wants to merge 9 commits into
--no-abi flag to sncast declare, declare-from & deploy#4269Conversation
This feature simply erases the ABI before computing the class hash, as implemented in `starkli`.
franciszekjob
left a comment
There was a problem hiding this comment.
Hey @die-herdplatte 👋 , thanks for submitting this PR - it looks good! Please take a look at the comments.
There was a problem hiding this comment.
When --no-abi is used, the declared class hash points to a class with an empty ABI. However, the generated deploy suggestion can still include --arguments, which requires sncast to fetch the ABI from that class hash in order to transform Cairo-like arguments into calldata. That suggested command will therefore fail or be misleading for classes with constructor args.
e.g. (constructor params x: felt252, y: felt252):
➜ no_abi_testing git:(main) ✗ ../target/debug/sncast declare --contract-name=HelloStarknet --no-abi --network sepolia
Compiling no_abi_testing v0.1.0 (/Users/franciszekjob/Projects/SWM/starknet-foundry/.worktrees/declare-no-abi/no_abi_testing/Scarb.toml)
Finished `release` profile target(s) in 0 seconds
Success: Declaration completed
Class Hash: 0x1ef0773957778f0060d0c276745679e144227d0343f6c91cd0f8a3f7713fda8
Transaction Hash: 0x1e7217a6799be480aff1384062c1bf67d39c11721c14871b8cc52643c90a053
To see declaration details, visit:
class: https://sepolia.voyager.online/class/0x01ef0773957778f0060d0c276745679e144227d0343f6c91cd0f8a3f7713fda8
transaction: https://sepolia.voyager.online/tx/0x01e7217a6799be480aff1384062c1bf67d39c11721c14871b8cc52643c90a053
To deploy a contract of this class, replace the placeholders in `--arguments` with your actual values, then run:
sncast deploy --class-hash 0x1ef0773957778f0060d0c276745679e144227d0343f6c91cd0f8a3f7713fda8 --arguments '<x: felt252>, <y: felt252>' --network sepolia
➜ no_abi_testing git:(main) ✗ ../target/debug/sncast deploy --class-hash 0x1ef0773957778f0060d0c276745679e144227d0343f6c91cd0f8a3f7713fda8 --arguments '10, 20' --url http://188.34.188.184:7070/rpc/v0_10
Error: Error while processing Cairo-like calldata
Caused by:
Invalid number of arguments: passed 2, expected 0I think we should either omit the --arguments hint when --no-abi is passed, or preferably switch the suggestion to raw --constructor-calldata placeholders, since raw calldata does not depend on the ABI.
There was a problem hiding this comment.
Similar problem occurs when --no-abi is used with deploy --contract-name ... --arguments ...
e.g.
➜ no_abi_testing git:(main) ✗ ../target/debug/sncast --no-abi --contract-name HelloStarknet --arguments '10, 20, 30' --network sepolia
Compiling no_abi_testing v0.1.0 (/Users/franciszekjob/Projects/SWM/starknet-foundry/.worktrees/declare-no-abi/no_abi_testing/Scarb.toml)
Finished `release` profile target(s) in 0 seconds
Error: Error while processing Cairo-like calldata
Caused by:
Invalid number of arguments: passed 3, expected 0| /// If passed, omits ABI from the declared Sierra class. This changes the resulting class hash | ||
| #[arg(long)] | ||
| pub no_abi: bool, | ||
|
|
There was a problem hiding this comment.
Keeping this in the common args works, but I think it may be clearer to move no_abi onto the concrete command structs instead, for example Declare and DeclareFrom.
In particular, for declare-from, putting the flag closer to the --sierra-file path would let us express the constraint more naturally and avoid this manual validation:
if declare_from.sierra_file.is_none() && declare_from.common.no_abi {
bail!("`--no-abi` can only be used with `--sierra-file`");
}| ## `--no-abi` | ||
|
|
||
| Optional. | ||
|
|
||
| If passed, omits ABI from the declared class. This changes the resulting class hash. | ||
|
|
There was a problem hiding this comment.
| ## `--no-abi` | |
| Optional. | |
| If passed, omits ABI from the declared class. This changes the resulting class hash. | |
| ## `--no-abi` | |
| Optional. | |
| If passed, omits ABI from the declared class. This changes the resulting class hash. | |
| Can only be used with `--sierra-file`. |
There was a problem hiding this comment.
I didn't add this because the network-mode-only arguments (--block-id, --source-url, --source-network) also don't have this hint
There was a problem hiding this comment.
I see. Could you please add them then?
There was a problem hiding this comment.
Let's add tests cases for:
deploy --contract-name --no-abi --arguments ...deploy --contract-name --no-abi --constructor-calldata ...
|
Hi @die-herdplatte, are you going to address the changes from review? |
|
Thanks for the review! |
…`sncast declare`
…ore a deploy Also fixes `--arguments` operating on an empty ABI when `--no-abi` is set
Allows clap to validate the mutual exclusivity
ddoktorski
left a comment
There was a problem hiding this comment.
Hi @die-herdplatte
Thanks for opening the PR! I've left a few comments that need to be addressed before merging 🙏
|
|
||
| /// Class hash of contract declared on a different Starknet instance | ||
| #[arg(short = 'g', long)] | ||
| #[arg(short = 'g', long, conflicts_with_all = ["no_abi"])] |
There was a problem hiding this comment.
| #[arg(short = 'g', long, conflicts_with_all = ["no_abi"])] | |
| #[arg(short = 'g', long, conflicts_with = "no_abi")] |
| use std::fs; | ||
| use test_case::test_case; | ||
|
|
||
| pub fn get_declared_class_hash_from_json_output(output: &[u8]) -> Felt { |
There was a problem hiding this comment.
Let's move this function to tests/helpers/output.rs (new file needs to be created)
| pub arguments: Option<String>, | ||
| } | ||
|
|
||
| enum AbiOrContractClass<C> { |
There was a problem hiding this comment.
Please remove this enum and add a function similar to:
fn abi_from_contract_class(contract_class: &ContractClass) -> Result<Vec<AbiEntry>> {
let ContractClass::Sierra(sierra_class) = contract_class else {
bail!("Transformation of arguments is not available for Cairo Zero contracts")
};
serde_json::from_str(sierra_class.abi.as_str())
.context("Couldn't deserialize ABI received from network")
}then replace all occurrences of AbiOrContractClass::ContractClass with a call to this function
| struct ConstructorArgs { | ||
| cairo_args: String, | ||
| typ: ConstructorArgsType, | ||
| } | ||
|
|
||
| #[derive(Clone, Serialize, Deserialize, Debug, PartialEq)] | ||
| enum ConstructorArgsType { | ||
| CairoLike, | ||
| RawCalldata, | ||
| } |
There was a problem hiding this comment.
It seems that it can be made as single enum:
| struct ConstructorArgs { | |
| cairo_args: String, | |
| typ: ConstructorArgsType, | |
| } | |
| #[derive(Clone, Serialize, Deserialize, Debug, PartialEq)] | |
| enum ConstructorArgsType { | |
| CairoLike, | |
| RawCalldata, | |
| } | |
| enum ConstructorArgs { | |
| CairoLike(String), | |
| RawCalldata(String), | |
| } |
| } | ||
|
|
||
| impl ConstructorArgs { | ||
| fn from_abi(abi: &[AbiEntry], no_abi: bool) -> Option<Self> { |
There was a problem hiding this comment.
| fn from_abi(abi: &[AbiEntry], no_abi: bool) -> Option<Self> { | |
| fn from_abi(abi: &[AbiEntry], abi_in_declared_class: bool) -> Option<Self> { |
Also requires relevant changes in the logic
This feature simply erases the ABI before computing the class hash, as implemented in
starkli.I didn't change the
declareinterface insncast_stdfor now since that would be a breaking change.